Move collect_bicolor_runs() to rustworkx-core
#1186
Conversation
| graph: G, | ||
| filter_fn: F, | ||
| color_fn: C, | ||
| ) -> Result<Vec<Vec<&<G as Data>::NodeWeight>>, CollectBicolorSimpleError<E>> //OG type: PyResult<Vec<Vec<PyObject>>> |
There was a problem hiding this comment.
Instead of returning NodeWeight here I think it would be simpler to just return something like:
impl Iterator<Item=Vec<G::NodeId>>
(or something like that I might have made a mistake in the exact syntax) Or alternatively a Vec<Vec<G::NodeId>>
The idea is to return an iterator of node indices for each run. This isn't exactly what the python api was returning, but retunring the node ids will let us easily iterate and map that to the weights for python, but for other rust consumer (especially Qiskit in the future) this will be lighter weight to work with because it's a simple vec get to go from an index to a weight.
Then using an iterator would be a more rust native interface to return an iterator of the runs
Pull Request Test Coverage Report for Build 9446628745Details
💛 - Coveralls |
ElePT
changed the title
collect_bicolor_runs() to rustworkx-corecollect_bicolor_runs() to rustworkx-core
mtreinish
added
the
rustworkx-core
…d unit tests and reno.
There was a problem hiding this comment.
This LGTM and I think it's ready to merge except for the missing assertion in the docstring example. There is an open question which I'm fine either way on which is whether the filter function and coloring functions are passed node weights or node ids, I can go either way so I'm curious your thoughts on it.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…s in tests for clarity.
|
I have tried out the suggestion of using ids instead of weights in c256e27, the only impact I noticed on the user side was the need to switch from one shared function for all tests to per-test closures in the unit tests, because a reference to the graph is needed to get the weights from the node/edge ids. I actually think this makes the tests more readable. I had hoped to be able to get rid of the result annotations, but the error type still needs to be determined, shame. |
rustworkx-core/src/dag_algo.rs
Outdated
| G: IntoNodeIdentifiers // Used in toposort | ||
| + IntoNeighborsDirected // Used in toposort | ||
| + IntoEdgesDirected // Used for .edges_directed | ||
| + IntoEdgeReferences |
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This is a first attempt at solving #1166, the PR introduces a new
collect_bicolor_runs()function inrustworx-coreand modifies the python-facingcollect_bicolor_runsto use the core function under the hood (no user-facing changes).The current function returns a
<Vec<Vec<G::NodeId>>, I could not find a way to implement the iterator mentioned in the original issue because I kept getting "the size cannot be known at compilation time", but I am open to implementation suggestions.